[fix] Attribute error when trying to get bounding box from a method field value by hemache · Pull Request #246 · openwisp/django-rest-framework-gis

hemache · 2020-11-05T16:18:51Z

fixes #247

…ield value

TravisBuddy · 2020-11-05T19:41:59Z

Hey @hemache,
Something went wrong with the build.

TravisCI finished with status errored, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.

View build log

TravisBuddy Request Identifier: f72e69c0-1f9e-11eb-bd2e-17a8c440c3de

TravisBuddy · 2020-11-06T00:10:48Z

Hey @hemache,
Something went wrong with the build.

TravisCI finished with status errored, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.

View build log

TravisBuddy Request Identifier: 84aa81b0-1fc4-11eb-9dab-419bf2960c30

nemesifier

Thanks for contributing @hemache, see my question below.

nemesifier · 2020-11-06T02:49:02Z

rest_framework_gis/serializers.py

+        if isinstance(field, SerializerMethodField):
+            method = getattr(field.parent, field.method_name)
+            geo_value = method(instance)
+            feature["geometry"] = field.to_representation(instance)


is using instance instead of geo_value intended?

is using instance instead of geo_value intended?

yes, because in this case, SerializerMethodField.to_representation receives object instance instead of the actual geometry value.

the problem was that geo_value initially stores the object instance (which doesn't provide .extent attribute), so here we're making sure to actually get the geometry value instead of the object instance.

ok, I don't understand then what's the purpose of calling geo_value = method(instance), it seems like that line can be removed, since geo_value is not used afterwards. Isn't it?
But I am also not entirely convinced of the approach as explained above.

since geo_value is not used afterwards. Isn't it?

yes, it's not used afterwards, but what if -later- someone else needed to use it?
I believe geo_value should have the correct/actual GEO value, not an instance of the object.

I don't understand this. I don't want to introduce changes in a library that is used by thousands of developers lightly. Each change must be justified.

@nemesisdesign it's justified. did you try to reproduce this bug?

focus on the type of geo_value variable. it should hold a GEO value and not something else, right?

@nemesisdesign if you're not convinced by my solution, then please try to reproduce the bug #247 and maybe suggest a proper fix?

TravisBuddy · 2020-11-06T03:32:58Z

Hey @hemache,
Something went wrong with the build.

TravisCI finished with status errored, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.

View build log

TravisBuddy Request Identifier: c2b92120-1fe0-11eb-9dab-419bf2960c30

hemache · 2020-11-06T10:43:09Z

@nemesisdesign any idea why the CI fails because it tries to access PostgreSQL?

atb00ker · 2020-11-08T10:27:10Z

@nemesisdesign any idea why the CI fails because it tries to access PostgreSQL?

@hemache Could you please try to change to dist: bionic in .travis.yml? It might help!

TravisBuddy · 2020-11-08T12:49:51Z

Hey @hemache,
Something went wrong with the build.

TravisCI finished with status errored, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.

View build log

TravisBuddy Request Identifier: e2c65be0-21c0-11eb-a04c-b76cbd6ddda3

atb00ker · 2020-11-08T13:18:59Z

Well, that didn't exactly help.

On inspection, I see:

Ver Cluster Port Status Owner    Data directory              Log file

12  main    5433 down   postgres /var/lib/postgresql/12/main /var/log/postgresql/postgresql-12-main.log

Perhaps cat /var/lib/postgresql/12/main /var/log/postgresql/postgresql-12-main.log would help. It should be a postgresql version problem, changing and experimenting with some other versions could help.

…er authentication for user "postgres"

hemache · 2020-11-11T20:29:49Z

fixed PostgreSQL issue.

but now I'm getting this error (e.g. https://travis-ci.com/github/openwisp/django-rest-framework-gis/jobs/433971220)

free(): invalid pointer
ERROR: InvocationError for command /home/travis/build/openwisp/django-rest-framework-gis/.tox/travis/bin/coverage run --source=rest_framework_gis ./tests/manage.py test tests/django_restframework_gis_tests (exited with code -6 (SIGABRT)) (exited with code -6)
___________________________________ summary ____________________________________
ERROR:   travis: commands failed
The command "tox -e travis" exited with 1.

though, I can't reproduce this in my local dev env

coverage run --source=rest_framework_gis ./tests/manage.py test tests/django_restframework_gis_tests
Creating test database for alias 'default'...
System check identified no issues (0 silenced).
............................s.........
GeoJSON rendering of 10000 objects completed in 6.299168238998391
.....s......./home/hemache/.local/share/virtualenvs/django-rest-framework-gis-EHv3zJno/lib/python3.8/site-packages/djangorestframework-3.12.1-py3.8.egg/rest_framework/pagination.py:200: UnorderedObjectListWarning: Pagination may yield inconsistent results with an unordered object_list: <class 'django_restframework_gis_tests.models.Location'> QuerySet.
  paginator = self.django_paginator_class(queryset, page_size)
............................
----------------------------------------------------------------------
Ran 79 tests in 63.766s

OK (skipped=2)
Destroying test database for alias 'default'...

any ideas?

nemesifier · 2020-11-11T21:23:43Z

@hemache did you already try using bionic as distro? Sometimes changing distro fixes these inexplicable issues.

hemache · 2020-11-12T09:10:56Z

@nemesisdesign yep, I tried that before

The following packages have unmet dependencies:
 gdal-bin : Depends: python3-gdal (= 3.0.4+dfsg-1~bionic0) but it is not going to be installed
            Depends: libgdal26 (>= 3.0.0) but it is not going to be installed
E: Unable to correct problems, you have held broken packages.
The command "sudo apt-get install binutils libproj-dev gdal-bin -y" failed and exited with 100 during .

https://travis-ci.com/github/openwisp/django-rest-framework-gis/jobs/434284716

can we please merge this anyway and fix CI issues later? (tests are passing btw)

hemache · 2020-12-01T15:42:22Z

@nemesisdesign any update on this, please? we're still using the patched version from my forked Github repo, I hope we can switch back to the official version in PyPi

atb00ker

Just one comment from my end,
I'll defer the rest to @nemesisdesign

.travis.yml

tests/django_restframework_gis_tests/urls.py

nemesifier

I am not sure of this change.
There's some repetition to check if the field is a SerializerMethodField.
I guess it could be resolved by changing the way SerializerMethodField and GeoFeatureModelSerializer interact.

I will look into it as soon as possible.

nemesifier · 2020-12-02T15:59:13Z

rest_framework_gis/serializers.py

+        if isinstance(field, SerializerMethodField):
+            method = getattr(field.parent, field.method_name)
+            geo_value = method(instance)
+            feature["geometry"] = field.to_representation(instance)


ok, I don't understand then what's the purpose of calling geo_value = method(instance), it seems like that line can be removed, since geo_value is not used afterwards. Isn't it?
But I am also not entirely convinced of the approach as explained above.

hemache · 2020-12-08T11:15:20Z

do you think this is ready to be merged?

auvipy · 2020-12-09T05:20:09Z

this need rebase

nemesifier · 2021-01-23T15:13:15Z

Not ready to merge: #246 (comment)

hemache added 2 commits November 5, 2020 17:15

[fix] attribute error when trying to get bounding box from a method f…

58ec8d0

…ield value

[qa] Test bounding box calculation when using method field for geo field

1d57899

[qa] Format code

191209b

nemesifier reviewed Nov 6, 2020

View reviewed changes

hemache force-pushed the fix-geoserializer-bbox-getter branch 3 times, most recently from b03cad0 to a52ae4b Compare November 11, 2020 18:45

[qa] Change PostgreSQL database port back to default value & Allow pe…

0040739

…er authentication for user "postgres"

hemache force-pushed the fix-geoserializer-bbox-getter branch from ebcba18 to 0040739 Compare November 11, 2020 19:09

hemache force-pushed the fix-geoserializer-bbox-getter branch from f7b09ed to 0040739 Compare November 12, 2020 09:11

nemesifier added the bug label Nov 13, 2020

atb00ker reviewed Dec 1, 2020

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

auvipy reviewed Dec 2, 2020

View reviewed changes

tests/django_restframework_gis_tests/urls.py Show resolved Hide resolved

[qa] Rollback CI PostgreSQL default port fix

84cd642

nemesifier reviewed Dec 2, 2020

View reviewed changes

nemesifier force-pushed the master branch from 081f767 to d137601 Compare May 9, 2022 22:58

nemesifier force-pushed the master branch from d137601 to f2dc812 Compare May 9, 2022 23:09

nemesifier removed this from OpenWISP Priorities for next releases Sep 30, 2025

Uh oh!

Conversation

hemache commented Nov 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TravisBuddy commented Nov 5, 2020

TravisBuddy Request Identifier: f72e69c0-1f9e-11eb-bd2e-17a8c440c3de

Uh oh!

TravisBuddy commented Nov 6, 2020

TravisBuddy Request Identifier: 84aa81b0-1fc4-11eb-9dab-419bf2960c30

Uh oh!

nemesifier left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TravisBuddy commented Nov 6, 2020

TravisBuddy Request Identifier: c2b92120-1fe0-11eb-9dab-419bf2960c30

Uh oh!

hemache commented Nov 6, 2020

Uh oh!

atb00ker commented Nov 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TravisBuddy commented Nov 8, 2020

TravisBuddy Request Identifier: e2c65be0-21c0-11eb-a04c-b76cbd6ddda3

Uh oh!

atb00ker commented Nov 8, 2020

Uh oh!

hemache commented Nov 11, 2020

Uh oh!

nemesifier commented Nov 11, 2020

Uh oh!

hemache commented Nov 12, 2020

Uh oh!

hemache commented Dec 1, 2020

Uh oh!

atb00ker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nemesifier left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hemache commented Dec 8, 2020

Uh oh!

auvipy commented Dec 9, 2020

Uh oh!

nemesifier commented Jan 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hemache commented Nov 5, 2020 •

edited

Loading

atb00ker commented Nov 8, 2020 •

edited

Loading